Add option to match belong_to with touch #222
Conversation
|
@panupan Thanks for the pull! I like this idea. The only thing that I do not like is the fact that if I have a model where I have specified I do see that it is handled this way in the validate option and I don't agree with that. Can you remove that please and we'll review from that point. |
|
Just wanted to check in and see if you had any updates on this. Please also rebase once you do since there have been many changes to master since this was last reviewed. Thanks! |
Also fixes validate failing when specified on model but not on matcher.
|
@drapergeek Hello, I fixed touch and validate to work how you suggested, I also added some tests to verify it. @mxie It should be up-to-date and good to go now. |
| @@ -258,10 +266,19 @@ def join_table_exists? | |||
| end | |||
|
|
|||
| def validate_correct? | |||
| if !@validate && !reflection.options[:validate] || @validate == reflection.options[:validate] | |||
| if !@options.key?(:validate) || @options[:validate] == (reflection.options[:validate] == true ? true : false) | |||
There was a problem hiding this comment.
The !@options.key?(:validate) change is good, but instead of:
@options[:validate] == (reflection.options[:validate] == true ? true : false)
I think you can just do:
@options[:validate] == reflection.options[:validate]
There was a problem hiding this comment.
Hi, that was breaking some tests. Because Rails defaults :validate to false so reflection.options[:validate] could be nil.
There was a problem hiding this comment.
Another option is:
@options[:validate] == !!reflection.options[:validate]
There was a problem hiding this comment.
@options[:validate] == !!reflection.options[:validate] is better, but perhaps we should add back in the check for reflection.options[:validate]?
There was a problem hiding this comment.
@mixie I'm not quite sure what adding that back in will work. It was causing the behavior drapergeek was describing above, where it would cause the matcher would fail if validate was specified in the model but not the matcher. BTW, I amended my last commit to use !!.
|
Hello, can someone review this again? It should be good to go. |
|
Thanks for this @panupan. 🚀 Also I really appreciate you fixing up the validate option as well. Good job! |
This option has been missing but is pretty much the same thing as matching the validate option. It could probably be refactored to prevent code duplication.